Skip to content

Konjit-w1-react #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

konjit
Copy link

@konjit konjit commented Apr 16, 2025

@tkitson tkitson self-assigned this Apr 17, 2025
Copy link

@tkitson tkitson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @konjit ! This is a great start, and its nearly there. Please find my suggestions below, and feel free to give me a shout on Slack if you have any questions. Thanks! Tom

key={category}
name={category}
selectedCategory={selectedCategory}
onClick={() => handleSubmit(category)}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works! But it doesn't really need the additional handleSubmit function - try just passing in setSelectedCategory to the onClick event. Also - there's a way that you could deselect the Category here as well, how could this be done? Think about using a ternary, and the 'category' variable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tkitson thank you so much for taking the time in to review the code and give further suggestions.

Indeed handleSubmit wasn't need and we toggle between select and deselect like this:

onClick={() =>
          setSelectedCategory(selectedCategory === category ? null : category)
  } 

}

.products-item {
flex: 0 0 400px;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works! Using flex-wrap is a nice way of pushing items onto the next line when they don't fit. But can you find a way of making the images scale in size as well? Try using media queries on the products-item class, and something like:

width: calc(50% - 16px);

You'll need to adjust the percentages, and check to see if you need the gap value in the products class above

}

.categories {
margin-left: 1rem;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a bit of space on the right here as well, for smaller devices like mobile

Copy link

@tkitson tkitson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @konjit well done on adding the toggling function! Working great, and I like the new hover state you have added to the listing items, nice touch. See how you get on with these comments and please let me know if you have any questions. Good luck! Tom

font-size: 1.1rem;
}

@media (max-width: 900px) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @konjit nice work! I think we can tidy this up and also help the styling further: Let's say we want to have a four-column layout at the widest viewport size, we can say:

@media (min-width: 1600px) { .products--item { width: calc(25% - 16px); } }

and then for three columns at a smaller screen it would be 33%, followed by 50% for two cols, then for mobile you could just have 100% and then wouldn't need to change the flex direction like you've done in line 103.

The '-16px' bit is to offset the gap that you can add to the '.product' class, lets say of 8px.

This should also help to centre the content - at the moment there is a big gap at the right hand side when the content isn't filling the full viewport width.

const Products = ({ selectedCategory }) => {
return (
<div className="products">
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think we could tidy this up a bit - see if you can move the logic outside of the render, and store it in a variable which is then mapped inside the component? E.g. const filteredProducts = !selectedCategory ? products : rest of logic here....

@konjit konjit requested a review from tkitson April 22, 2025 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants